Implement delete_objs in fs and s3 storage backends. #395
Conversation
zijie0
changed the title
rust/src/storage/file/mod.rs
Outdated
| @@ -134,6 +134,17 @@ impl StorageBackend for FileStorageBackend { | |||
| async fn delete_obj(&self, path: &str) -> Result<(), StorageError> { | |||
| fs::remove_file(path).await.map_err(StorageError::from) | |||
| } | |||
|
|
|||
| async fn delete_objs(&self, paths: &[String]) -> Result<(), StorageError> { | |||
There was a problem hiding this comment.
I think paths: &[&str] would be a slightly better signature.
There was a problem hiding this comment.
I originally used &[&str] but later met some problem when calling this method from vacuum... It said that: temporary value created here. So I think maybe we should use String instead of &str here?
There was a problem hiding this comment.
Ideally, I think the method should be defined as a generic function taking impl Iterator<AsRef<str>> as input. But this trait is being used as trait object, so we can't use generic here, which makes things really annoying. I also think &[&str] would have been a cleaner interface, but it will force us to allocate both Vec<String> and Vec<&str> in vacuum method, which has performance overheads :( I also don't really have a good suggestion on how to workaround this issue here.
There was a problem hiding this comment.
@Dandandan Do you have any suggestion on this? Or we could go ahead with &[String] for now?
|
I added some comments @zijie0 |
|
@Dandandan Thank you for your suggestion, learned a lot from your PR :) |
* Batch delete in chunk. * Add default implementation for `delete_objs`.
rtyler
added
binding/rust
|
I think this is a great improvement. I am wondering a bit how errors in the S3 implementation are handled, as the But I don't think that should be covered in this PR. |
|
Thank you both for this new optimization @zijie0 @Dandandan ! |
Description
Implement delete_objs in fs and s3 storage backends. Add corresponding test cases.
Related Issue(s)
Documentation